feat/ add log forwarding in HadesLogManager and ContinueOnError step support#360
feat/ add log forwarding in HadesLogManager and ContinueOnError step support#360paoxin wants to merge 41 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduces HadesLogManager service for monitoring job logs and syncing them to an external API endpoint. Adds per-job watcher lifecycle synchronization via WaitGroup, HTTP endpoints for log/status retrieval, step-level error handling with optional continuation on failure, and deploys the service across Docker Compose environments with necessary configuration and CI/CD integration. Changes
Sequence Diagram(s)sequenceDiagram
participant Scheduler as HadesScheduler
participant LogMgr as HadesLogManager
participant NATS as NATS Broker
participant API as Artemis API
Scheduler->>NATS: Publish job.running event
NATS->>LogMgr: Receive job.running
LogMgr->>LogMgr: Create watcher with WaitGroup
LogMgr->>NATS: Subscribe to job logs
loop Streaming Logs
NATS->>LogMgr: Publish log entries
LogMgr->>LogMgr: Accumulate logs in memory
end
Scheduler->>NATS: Publish job.completed event
NATS->>LogMgr: Receive job.completed
LogMgr->>LogMgr: Wait for watcher completion
LogMgr->>LogMgr: Marshal logs to JSON
LogMgr->>API: POST /adapter/logs (10s timeout)
API->>LogMgr: 200 OK
LogMgr->>LogMgr: Mark job completed
LogMgr->>LogMgr: Cleanup watcher entry
sequenceDiagram
participant Job as Job.execute()
participant Step as Step Processing
participant StepExec as Container Execution
Job->>Job: Initialize stepErr = nil
loop For each step
Job->>Step: Get step with UUID
Job->>StepExec: execute(ctx, step)
StepExec->>StepExec: defer cleanup container
alt Step Success
StepExec-->>Job: nil
else Step Fails
StepExec-->>Job: error
alt ContinueOnError = true
Job->>Job: errors.Join(stepErr, stepErr)
Job->>Job: Log condition, continue
else ContinueOnError = false
Job->>Job: Return error immediately
end
end
end
Job->>Job: Return accumulated stepErr
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 4❌ Failed checks (2 warnings, 2 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
HadesLogManager/log_subscriber.go (1)
224-235:⚠️ Potential issue | 🔴 CriticalBlocking HTTP call while holding mutex; error ignored.
SendJobLogsperforms a synchronous HTTP POST (with no timeout on the client, as noted in processor.go). This call is made whiledlm.muis held, which blocks all concurrentstartWatchingJobLogsandstopWatchingJobLogscalls for the duration of the network round-trip. If the adapter is down or slow, this effectively deadlocks the log manager.Additionally, the returned error is silently discarded.
🐛 Proposed fix: move the send outside the lock and handle the error
func (dlm *DynamicLogManager) stopWatchingJobLogs(jobID string) { dlm.mu.Lock() - defer dlm.mu.Unlock() - + var shouldSend bool if watcher, exists := dlm.watchers[jobID]; exists { delete(dlm.watchers, jobID) slog.Info("Stopping log watch", "job_id", jobID) watcher.cancel() - dlm.logAggregator.MarkJobCompleted(jobID) - dlm.logAggregator.SendJobLogs(jobID) + shouldSend = true + } + dlm.mu.Unlock() + + if shouldSend { + if err := dlm.logAggregator.SendJobLogs(jobID); err != nil { + slog.Error("Failed to send job logs", "job_id", jobID, "error", err) + } } }
🤖 Fix all issues with AI agents
In `@HadesArtemisAdapter/artemis_adapter.go`:
- Around line 93-122: checkAndSendIfReady can run concurrently from StoreLogs
and StoreResults and cause duplicate sends because aa.logs.Load and
aa.results.Load are not atomic with the subsequent deletes; protect the entire
check-and-send-and-delete sequence with mutual exclusion. Add a per-job mutex
map (or a single mutex) on ArtemisAdapter (e.g., a field like jobLocks
map[string]*sync.Mutex or a sync.Mutex lock) and acquire the appropriate lock at
the start of checkAndSendIfReady (or have StoreLogs/StoreResults lock per job
before calling it) so the load, conditional, sendToArtemis(results) and
aa.logs.Delete/aa.results.Delete execute under the same lock, then release the
lock after cleanup to ensure exactly-once delivery for a jobID.
- Around line 125-149: The sendToArtemis function currently logs errors but
continues, leading to nil-pointer dereferences and always returning nil; update
sendToArtemis (in artemis_adapter.go) to check and return errors immediately
after json.Marshal, http.NewRequest, and aa.httpClient.Do (wrap each error with
context, e.g., fmt.Errorf("marshal request: %w", err)), only call defer
resp.Body.Close() after confirming resp != nil, and return the error to callers
instead of nil so checkAndSendIfReady can detect failures; ensure success path
still returns nil.
- Around line 65-68: Replace the hardcoded Artemis constants with configuration
loaded from environment/config to avoid committing secrets and localhost URLs:
remove the literal artemisAuthToken and artemisBaseURL values and read them from
env/config (e.g., ARTEMIS_BASE_URL, ARTEMIS_AUTH_TOKEN) into the adapter's
config struct (same pattern as other files using `env` tags), keep
newResultEndpoint as a constant path if desired but compose full URL from the
configured base, and move requestTimeout into the config (or use a sensible
default) so no secrets or deploy-specific URLs remain in
artemisBaseURL/artemisAuthToken in artemis_adapter.go.
- Around line 78-84: StoreLogs currently assumes logs[1] exists and does
execution_logs := logs[1].Logs which panics if logs has fewer than 2 entries;
update StoreLogs to check slice length and nil/empty elements before accessing
index 1, return a clear error if execution logs are missing (or skip storing and
return nil depending on caller expectations), and rename execution_logs to
executionLogs to follow Go naming conventions; ensure you still call
aa.logs.Store(jobID, executionLogs) and return aa.checkAndSendIfReady(jobID)
only after a successful bounds-checked extraction.
In `@HadesArtemisAdapter/go.mod`:
- Line 36: The project currently pins the indirect dependency
"github.com/quic-go/quic-go v0.54.0" which has HIGH-severity CVEs; update the
dependency to v0.57.0 or later by replacing the version entry for
github.com/quic-go/quic-go in go.mod and then run go get
github.com/quic-go/quic-go@v0.57.0 (or a newer stable release) followed by go
mod tidy to refresh the module graph and vendor files; verify no other
dependency requires the older version and run your test suite to confirm
compatibility.
- Around line 1-3: Update the module declaration in go.mod to match the
repository naming convention by replacing the current module name
`Hades-ArtemisAdapter` with `github.com/ls1intum/hades/artemis-adapter`; ensure
imports across the module (packages referencing this module) are updated to the
new path if needed and keep the existing Go version (go 1.25.3) unchanged.
In `@HadesArtemisAdapter/main.go`:
- Around line 138-148: The POST /test-results handler currently calls
aa.StoreResults(newResults.UUID, newResults) and ignores its returned error, so
failures (including from sendToArtemis) still return 201; update the handler to
capture the error from StoreResults, log it (slog.Error) with context (uuid and
error), and respond with an appropriate HTTP status (e.g., 500) and error
message when non-nil; keep successful behavior unchanged (log debug and return
201 with the payload).
- Around line 100-102: The 500ms shutdown timeout in the context.WithTimeout
call (where shutdownCtx, shutdownCancel are created) is too short and will kill
in-flight requests like sendToArtemis; change the timeout to a more reasonable
value (e.g., 15–30 seconds) by replacing 500*time.Millisecond with a longer
duration (suggest 30*time.Second) or wire a configurable ShutdownTimeout
constant/flag so main.go uses the same 30s policy as HadesLogManager when
calling context.WithTimeout to create shutdownCtx and shutdownCancel.
- Around line 125-135: The handler reading newLogs via c.BindJSON can panic
because it accesses newLogs[0].JobID without checking the slice length and it
ignores the error returned by aa.StoreLogs; update the POST "/logs" handler to
validate that newLogs is non-empty after BindJSON (return a 400/422 with a
logged error if empty), use newLogs[0].JobID only after that check, call
aa.StoreLogs(...) and handle its returned error (log it and return a
500/appropriate status on failure), and ensure the response (c.IndentedJSON) is
only sent after successful storage; refer to the variables/funcs newLogs,
c.BindJSON, aa.StoreLogs, and c.IndentedJSON to locate the change.
In `@HadesLogManager/processor.go`:
- Line 17: The constant APIendpoint in processor.go is hardcoded to
"http://localhost:8082/adapter/logs" which breaks non-local deployments; change
it to be configurable via an environment-driven config (e.g., add a field to
AggregatorConfig such as LogsAPIEndpoint or create a dedicated config struct)
and replace usages of APIendpoint with that config field, initializing the value
from an environment variable (with a sensible default) during program startup.
- Around line 199-233: In SendJobLogs, stop continuing after failures: after
json.Marshal(logs) return the error (with a logged message) if err != nil; after
http.NewRequest("POST", APIendpoint, ...) return the error if err != nil instead
of proceeding to use req; after client.Do(req) check resp and err and if err !=
nil return it (after logging) and only then use resp; move defer
resp.Body.Close() immediately after the nil-check that confirms resp != nil; and
ensure SendJobLogs returns non-nil errors (propagate or wrap them) instead of
always returning nil so callers can handle failures.
🧹 Nitpick comments (4)
HadesScheduler/docker/job.go (1)
31-31: Inconsistent naming: env var"UUID"vs context key"job_id".The environment variable is called
"UUID"(line 31) while the context key is"job_id"(line 42). Consider aligning these — e.g., use"JOB_ID"(or"HADES_JOB_ID") for the env var to reduce ambiguity and avoid collisions with the very generic name"UUID".Suggested diff
- envs["UUID"] = d.ID.String() + envs["JOB_ID"] = d.ID.String()Also applies to: 42-42
docs/api/Create Build Job (Test Succeed) with Result Parse Step.bru (1)
75-78:host.docker.internalmay not resolve on Linux Docker hosts.This hostname works out-of-the-box on Docker Desktop (macOS/Windows) but requires
--add-host=host.docker.internal:host-gatewayon Linux. Consider adding a comment or using a Bruno variable (e.g.,{{adapter_host}}) so the endpoint is easily configurable across environments.♻️ Suggested change
"metadata": { - "API_ENDPOINT": "http://host.docker.internal:8082/adapter/test-results", + "API_ENDPOINT": "http://{{adapter_host}}/adapter/test-results", "INGEST_DIR": "/shared/example/build/test-results/test" }Then add
adapter_hostto thevars:pre-requestsection:vars:pre-request { user: password: test_repo: https://github.com/Mtze/Artemis-Java-Test.git assignment_repo: https://github.com/Mtze/Artemis-Java-Solution.git + adapter_host: host.docker.internal:8082 }HadesLogManager/processor.go (1)
219-219:http.Clientcreated per call with no timeout.A new
http.Client{}is instantiated on everySendJobLogscall with zero timeout (waits forever). Reuse a client stored on the struct and set an explicit timeout.HadesArtemisAdapter/artemis_adapter.go (1)
70-76:ctxparameter is unused inNewAdapter.The
context.Contextparameter is accepted but never used. Either remove it or wire it into the HTTP client (e.g., for cancellation support on in-flight requests).
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @.env.example:
- Line 28: The environment variable name in .env.example (ARTEMIS_ADAPTER_PORT)
doesn't match the struct tag read by AdapterConfig in
HadesArtemisAdapter/main.go (env:"API_PORT"); update .env.example to rename
ARTEMIS_ADAPTER_PORT=8082 to API_PORT=8082 so AdapterConfig picks it up, or
alternatively change the AdapterConfig struct tag from env:"API_PORT" to
env:"ARTEMIS_ADAPTER_PORT" to keep the current .env.example—choose one
consistent fix and apply it to ensure the env name and AdapterConfig match.
In `@HadesArtemisAdapter/artemis_adapter.go`:
- Around line 78-90: StoreLogs currently logs an Error but proceeds to store an
empty executionLogs and calls checkAndSendIfReady, causing results to be sent
without logs; update StoreLogs to not proceed when execution logs are missing:
inside StoreLogs (function StoreLogs) when len(logs) < 2, either (preferred)
return a descriptive error (e.g., fmt.Errorf("missing execution logs for job
%s", jobID)) and do not call aa.logs.Store or checkAndSendIfReady, keeping or
strengthening the Error log, or if degraded behavior is intentional change the
slog.Error to slog.Warn and return nil as before; ensure the
aa.logs.Store(jobID, ...) and call to checkAndSendIfReady(jobID) only happen in
the successful branch where executionLogs is populated.
In `@HadesArtemisAdapter/main.go`:
- Around line 131-136: The error message in the /logs handler is a copy-paste:
when binding into newLogs (type []buildlogs.Log) the slog.Error call uses
"Failed to bind test results JSON"; update that log to accurately reflect this
handler (e.g., "Failed to bind logs JSON") so the message matches the endpoint
and variable names (jobs.POST("/logs" handler, newLogs, c.BindJSON, slog.Error).
- Around line 34-35: LoadConfig's returned error is being ignored which can
leave AdapterConfig (cfg) zero-valued; change the call to capture and handle the
error from utils.LoadConfig(&cfg) in main: call err := utils.LoadConfig(&cfg)
and if err != nil log the error (including err) and exit (or return) so we don't
continue with empty fields like ArtemisBaseURL or ArtemisAuthToken; reference
symbols: AdapterConfig, utils.LoadConfig, cfg, ArtemisBaseURL, ArtemisAuthToken.
In `@HadesLogManager/processor.go`:
- Around line 202-227: SendJobLogs currently treats any HTTP response as success
because it never checks resp.StatusCode; update SendJobLogs to inspect
resp.StatusCode after client.Do(req) and treat non-2xx codes as errors by
reading resp.Body (to include adapter error details) and returning a formatted
error (e.g., include status code and body). Keep the existing defer
resp.Body.Close(), but add a small read of resp.Body (use ioutil.ReadAll or
io.ReadAll) for the error case and return fmt.Errorf with both resp.StatusCode
and the response body string; only return nil when resp.StatusCode is in the
200–299 range.
🧹 Nitpick comments (4)
.env.example (1)
34-34: Double quotes inside the value will be treated as literal characters by some dotenv loaders.The
godotenvlibrary (used viashared/utils/config.go) strips surrounding quotes, so this specific case should be fine. However, the static analysis hint flags this as potentially confusing. For consistency with other unquoted entries (like line 29–31), consider removing the quotes.Proposed fix
-API_ENDPOINT="http://localhost:8082/adapter/logs" +API_ENDPOINT=http://localhost:8082/adapter/logsHadesLogManager/processor.go (2)
209-209: Logging the full JSON payload at Debug level can be expensive for large log sets.
string(jsonData)forces allocation of the entire serialized payload into a log line. Consider logging only the byte length instead, or gating behind a more verbose level.Proposed fix
- slog.Debug("Parsed logs to JSON", "json", string(jsonData)) + slog.Debug("Marshaled logs to JSON", "job_id", jobID, "bytes", len(jsonData))
50-56: Minor:APIendpointfield should follow Go naming conventions →APIEndpoint.Go convention treats acronyms as single words (
API), andEndpointis a separate word, so the idiomatic name isAPIEndpoint.HadesArtemisAdapter/artemis_adapter.go (1)
69-76:ctxparameter is accepted but unused.
NewAdapteraccepts acontext.Contextbut neither stores it nor passes it onward. Either use it (e.g., for request contexts insendToArtemisviahttp.NewRequestWithContext) or remove it to avoid a misleading API.
* HadesLogManager to use refactored interface *go mod tidy
Mirrors the Docker executor's continue-past-failure behavior: the init container script now appends "; exit 0" when the step is marked continueOnError, and the CRD schema is regenerated via make manifests so the field isn't pruned by the API server.
✨ What is the change?
ContinueOnErrorfield to HadesAPI Step struct: to tell the Scheduler whether to continue the next execution step if the current step fails.these changes are made in accordance to the adapter: https://github.com/ls1intum/hades-artemis-adapter
📌 Reason for the change / Link to issue
Closes #269 #261 #270
🧪 Steps for Testing
✅ PR Checklist
Summary by CodeRabbit
New Features
Documentation
Chores